src/utils/icons/reactIcons.ts (3)

50-54: Static analysis ReDoS warning is a false positive in this context.

The regex at line 51 is constructed from setId values that originate from IconsManifest (react-icons library internal data), not from user input. The pattern ^${setPrefix} is a simple anchored prefix match without problematic quantifiers, making ReDoS practically impossible here.

However, if you want to be extra defensive, you could sanitize the input or use a literal string match instead:

♻️ Optional: Use string operations instead of regex

   } else {
     const setPrefix = setId.charAt(0).toUpperCase() + setId.slice(1)
-    const prefix = new RegExp(`^${setPrefix}`, 'i')
-    base = base.replace(prefix, '')
+    if (base.toLowerCase().startsWith(setPrefix.toLowerCase())) {
+      base = base.slice(setPrefix.length)
+    }
   }

---

82-84: Consider logging failed icon set loads for debugging.

Silent failures make troubleshooting difficult. While graceful degradation is appropriate, logging would help identify issues during development or when icon sets are unexpectedly unavailable.

♻️ Optional: Add debug logging

-    } catch {
-      // Skip sets that fail to load
+    } catch (error) {
+      // Skip sets that fail to load, but log for debugging
+      console.debug(`Failed to load icon set '${setId}':`, error)
     }

---

62-88: Consider caching getAllIcons() result to avoid repeated dynamic imports.

getAllIcons() performs dynamic imports for every icon set on each call. For a production API, this could be expensive if called frequently. Consider memoizing the result since icon sets don't change at runtime.

♻️ Proposed caching implementation

+let cachedIcons: IconMetadata[] | null = null
+
 export async function getAllIcons(): Promise<IconMetadata[]> {
+  if (cachedIcons) {
+    return cachedIcons
+  }
+
   const icons: IconMetadata[] = []

   for (const setId of ICON_SET_IDS) {
     // ... existing code ...
   }

+  cachedIcons = icons
   return icons
 }

src/__tests__/pages/api/__tests__/[version]/icons/[iconName].test.ts (1)

17-24: Mock duplicates production logic - consider importing the real function.

The parseIconId mock reimplements the actual logic, which could lead to test/implementation drift. Consider using Jest's requireActual to import the real function while mocking only what's necessary.

♻️ Proposed: Use real parseIconId implementation

 jest.mock('../../../../../../utils/icons/reactIcons', () => ({
   getIconSvg: jest.fn((setId: string, iconName: string) => {
     if (setId === 'fa' && iconName === 'FaCircle') return Promise.resolve(mockSvg)
     return Promise.resolve(null)
   }),
-  parseIconId: jest.fn((iconId: string) => {
-    const underscoreIndex = iconId.indexOf('_')
-    if (underscoreIndex <= 0) return null
-    const setId = iconId.slice(0, underscoreIndex)
-    const iconName = iconId.slice(underscoreIndex + 1)
-    if (!setId || !iconName) return null
-    return { setId, iconName }
-  }),
+  parseIconId: jest.requireActual('../../../../../../utils/icons/reactIcons').parseIconId,
 }))

src/pages/api/openapi.json.ts (2)

107-163: Consider adding 404 response for version not found.

The /{version}/icons endpoint returns 404 when the version doesn't exist (per the implementation in src/pages/api/[version]/icons/index.ts), but this isn't documented in the OpenAPI spec.

📝 Add 404 response documentation

             '200': {
               description: 'List of icons with metadata',
               // ... existing schema ...
             },
+            '404': {
+              description: 'Version not found',
+            },
           },

---

164-201: Consider adding 400 response for invalid icon name format.

The /{version}/icons/{icon-name} endpoint returns 400 for invalid icon name formats, but only 404 is documented. This could cause confusion for API consumers.

📝 Add 400 response documentation

           responses: {
             '200': {
               description: 'SVG markup for the icon',
               content: {
                 'image/svg+xml': {
                   schema: { type: 'string' },
                 },
               },
             },
+            '400': {
+              description: 'Invalid icon name format. Expected: {set}_{iconName}',
+            },
             '404': {
               description: 'Icon not found',
             },
           },

src/pages/api/[version]/icons/[iconName].ts (1)

17-17: Minor: Variable aliasing may cause confusion.

The destructuring { version, iconName: iconId } creates an alias, but iconName is reused later (line 53) as a different variable from parseIconId. While not a bug, this could be clearer.

♻️ Suggested clarification

Consider renaming for clarity:

-  const { version, iconName: iconId } = params
+  const { version, iconName: rawIconId } = params

And update references to iconId → rawIconId accordingly.

src/__tests__/pages/api/__tests__/[version]/icons/index.test.ts (1)

50-51: Consider using describe block and afterEach for cleaner test organization.

Tests would benefit from wrapping in a describe block and moving the repeated jest.restoreAllMocks() calls to an afterEach hook.

♻️ Suggested structure

+describe('GET /api/{version}/icons', () => {
+  afterEach(() => {
+    jest.restoreAllMocks()
+  })
+
 it('returns all icons with metadata', async () => {
   global.fetch = jest.fn(() =>
     ...
   expect(body.icons[0]).toHaveProperty('unicode')
-
-  jest.restoreAllMocks()
 })
 // ... other tests without individual restoreAllMocks calls
+})

Also applies to: 79-80

src/pages/api/[version]/icons/index.ts (1)

32-33: Consider pagination for large icon sets.

react-icons contains thousands of icons across all sets. Returning all icons in a single response could impact performance and client-side rendering. Consider adding pagination (limit/offset or cursor-based) for production use.

src/__tests__/pages/api/__tests__/[version]/icons/index.test.ts (1)

62-87: Consider cleaning up global.fetch in an afterEach block.

jest.restoreAllMocks() only restores mocks created via jest.spyOn, not direct global.fetch assignments. If a test throws before the restoreAllMocks call, the mutated global.fetch leaks. Since each test reassigns it anyway this is low risk, but an afterEach would be cleaner.

♻️ Suggested cleanup pattern

+const originalFetch = global.fetch
+
+afterEach(() => {
+  global.fetch = originalFetch
+  jest.restoreAllMocks()
+})
+
 it('returns all icons with metadata', async () => {
   global.fetch = createFetchMock()
   // ...
-  jest.restoreAllMocks()
 })

src/utils/icons/reactIcons.ts (1)

117-145: Consider the memory and build-time cost of rendering all icons in a set.

getIconSvgsForSet renders every icon in a set to SVG at build time via renderToStaticMarkup. For large sets (e.g., Material Design has ~10,000+ icons), this could be memory-intensive during the build. Since this runs at prerender time (not in Workers), it should be fine, but worth noting if build times become a concern.

src/pages/iconsIndex.json.ts (1)

10-30: Inconsistent response construction — use createJsonResponse for uniformity.

Both the success and error paths construct new Response manually, missing the Cache-Control, Date, and Content-Length headers that createJsonResponse provides. The error path also uses String(error) (which prepends "Error: " for Error instances) rather than the error instanceof Error ? error.message : String(error) pattern used in sibling routes.

♻️ Suggested refactor

+import { createJsonResponse } from '../utils/apiHelpers'

 export const GET: APIRoute = async () => {
   try {
     const icons = await getAllIcons()
-    return new Response(JSON.stringify({ icons }), {
-      status: 200,
-      headers: { 'Content-Type': 'application/json' },
-    })
+    return createJsonResponse({ icons })
   } catch (error) {
-    return new Response(
-      JSON.stringify({ error: 'Failed to load icons index', details: String(error) }),
-      { status: 500, headers: { 'Content-Type': 'application/json' } }
-    )
+    const details = error instanceof Error ? error.message : String(error)
+    return createJsonResponse({ error: 'Failed to load icons index', details }, 500)
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/iconsIndex.json.ts` around lines 10 - 30, The GET handler currently
returns manual Response objects (wrapping getAllIcons) and on error uses
String(error); replace both success and catch branches to call the shared
createJsonResponse helper so responses include standardized headers
(Cache-Control, Date, Content-Length) and consistent serialization, and change
the error serialization to use error instanceof Error ? error.message :
String(error); update the success branch to return createJsonResponse({ icons })
and the catch branch to return createJsonResponse({ error: 'Failed to load icons
index', details: error instanceof Error ? error.message : String(error) }, 500),
referencing the GET export and getAllIcons call.

src/pages/api/[version]/icons/[setId].json.ts (1)

13-18: flatMap with a single-element array is equivalent to map here.

If the intent is to expand to multiple setId values per version in the future, a comment would clarify the choice; otherwise, simplify:

♻️ Suggested simplification

-  return versions.flatMap((version) => [
-    { params: { version, setId: 'pf' } },
-  ])
+  return versions.map((version) => ({ params: { version, setId: 'pf' } }))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/`[version]/icons/[setId].json.ts around lines 13 - 18, The
getStaticPaths function uses versions.flatMap returning a single-element array,
which is equivalent to map; replace flatMap with map and return the object
directly (e.g., versions.map(version => ({ params: { version, setId: 'pf' } })))
to simplify, or if you intentionally plan to expand to multiple setId values
later, keep flatMap but add a clarifying comment above getStaticPaths noting
that flatMap is used to support multiple setId entries per version in the
future; reference getStaticPaths, versions, and the setId 'pf' when making the
change.

src/utils/apiIndex/get.ts (1)

63-92: Extract shared file-reading helper to eliminate duplication with getApiIndex.

getVersionsFromIndexFile repeats the same readFile / JSON.parse / ENOENT / SyntaxError boilerplate already present in getApiIndex. A small private helper (e.g. readAndParseIndexFile) would let both callers share the I/O and error-wrapping logic while preserving their individual validation paths.

♻️ Suggested refactor

+async function readAndParseIndexFile(indexPath: string): Promise<unknown> {
+  try {
+    const content = await readFile(indexPath, 'utf-8')
+    return JSON.parse(content)
+  } catch (error) {
+    if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
+      throw new Error(
+        `API index file not found at ${indexPath}. ` +
+          'Please run the build process to generate the index.',
+      )
+    }
+    if (error instanceof SyntaxError) {
+      throw new Error(
+        `API index contains invalid JSON at ${indexPath}. Please rebuild to regenerate the index file.`,
+      )
+    }
+    throw error
+  }
+}

 export async function getApiIndex(): Promise<ApiIndex> {
   const outputDir = await getOutputDir()
   const indexPath = join(outputDir, 'apiIndex.json')
-  try {
-    const content = await readFile(indexPath, 'utf-8')
-    const parsed = JSON.parse(content)
+  const parsed = await readAndParseIndexFile(indexPath)
     // Validate index structure
     ...
-  } catch (error) { ... }
 }

 export async function getVersionsFromIndexFile(): Promise<string[]> {
   const outputDir = await getOutputDir()
   const indexPath = join(outputDir, 'apiIndex.json')
-  try {
-    const content = await readFile(indexPath, 'utf-8')
-    const parsed = JSON.parse(content)
+  const parsed = await readAndParseIndexFile(indexPath)
   if (!parsed.versions || !Array.isArray((parsed as any).versions)) { ... }
-  } catch (error) { ... }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/apiIndex/get.ts` around lines 63 - 92, Extract the duplicated
file-read/parse/error-handling logic into a small private helper (e.g.,
readAndParseIndexFile) and have both getVersionsFromIndexFile and getApiIndex
call it: move the await readFile(indexPath, 'utf-8'), JSON.parse, and the ENOENT
/ SyntaxError mapping into readAndParseIndexFile so it returns the parsed object
(or throws the same contextual errors), then update getVersionsFromIndexFile to
call readAndParseIndexFile and only perform the versions-array validation, and
update getApiIndex to call the helper and keep its own specific validation;
ensure helper uses the same indexPath construction and preserves thrown error
messages.

src/utils/icons/fetch.ts (1)

47-63: Inconsistent error model between fetchIconsIndex (throws) and fetchIconSvgs (returns null).

fetchIconSvgs returns null on non-OK HTTP responses but can still throw on network failures. Callers therefore need to handle both patterns. Aligning both functions to throw on any failure (and letting callers decide how to surface errors) would simplify usage and prevent the silent-null footgun seen in [iconName].ts.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/icons/fetch.ts` around lines 47 - 63, The function fetchIconSvgs
currently returns null on non-OK HTTP responses which makes its error model
inconsistent with fetchIconsIndex; change fetchIconSvgs to throw an Error on any
non-OK response (include response.status and response.statusText or response
body for context) instead of returning null, and update its return type to
Promise<Record<string, string>> so callers only handle thrown errors; keep
existing behavior for network exceptions (let them propagate) and reference the
function name fetchIconSvgs and the response/iconsSvgsUrl variables when making
the change.

src/utils/icons/reactIcons.ts (2)

50-122: getAllIcons and getIconSvgsForSet duplicate directory-reading logic.

Both functions resolve the static dir, check existence, read the directory, filter .svg files, and compute reactName. The only meaningful difference is what they accumulate. Extract a private helper to avoid this repetition:

♻️ Proposed refactor

+interface SvgFileEntry {
+  kebab: string
+  reactName: string
+  filePath: string
+}
+
+function readSvgEntries(): SvgFileEntry[] {
+  const staticDir = getStaticIconsDir()
+  if (!fs.existsSync(staticDir)) {
+    return []
+  }
+  return fs
+    .readdirSync(staticDir)
+    .filter((f) => f.endsWith('.svg'))
+    .map((file) => {
+      const kebab = file.replace(/\.svg$/, '')
+      return { kebab, reactName: kebabToReactName(kebab), filePath: path.join(staticDir, file) }
+    })
+}
+
 export async function getAllIcons(): Promise<IconMetadata[]> {
-  const staticDir = getStaticIconsDir()
-  if (!fs.existsSync(staticDir)) {
-    return []
-  }
-  const files = fs.readdirSync(staticDir)
-  const svgFiles = files.filter((f) => f.endsWith('.svg'))
-  const icons: IconMetadata[] = []
-  for (const file of svgFiles) {
-    const name = file.replace(/\.svg$/, '')
-    const reactName = kebabToReactName(name)
-    icons.push({
-      name,
-      reactName,
-      usage: `import { ${reactName} } from '@patternfly/react-icons'`,
-    })
-  }
-  return icons
+  return readSvgEntries().map(({ kebab, reactName }) => ({
+    name: kebab,
+    reactName,
+    usage: `import { ${reactName} } from '@patternfly/react-icons'`,
+  }))
 }

 export async function getIconSvgsForSet(setId: string): Promise<Record<string, string>> {
   if (setId !== PF_ICONS_SET_ID) {
     return {}
   }
-  const staticDir = getStaticIconsDir()
-  if (!fs.existsSync(staticDir)) {
-    return {}
-  }
-  const files = fs.readdirSync(staticDir)
-  const svgFiles = files.filter((f) => f.endsWith('.svg'))
   const svgs: Record<string, string> = {}
-  for (const file of svgFiles) {
-    const name = file.replace(/\.svg$/, '')
-    const reactName = kebabToReactName(name)
-    const filePath = path.join(staticDir, file)
-    const content = fs.readFileSync(filePath, 'utf-8')
-    svgs[reactName] = content.trim()
-  }
+  for (const { reactName, filePath } of readSvgEntries()) {
+    svgs[reactName] = fs.readFileSync(filePath, 'utf-8').trim()
+  }
   return svgs
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/icons/reactIcons.ts` around lines 50 - 122, Both getAllIcons and
getIconSvgsForSet duplicate the same directory-checking and SVG-file parsing
logic; extract a private helper (e.g., gatherStaticSvgFiles or listStaticSvgs)
that accepts no args or a callback and returns an array of objects like { name,
reactName, filePath } (or returns names and let caller read content), then
update getAllIcons to map that array to IconMetadata (usage string) and update
getIconSvgsForSet to read file contents from filePath and build the
Record<string,string>; ensure the helper uses getStaticIconsDir, fs.existsSync,
fs.readdirSync and kebabToReactName so callers no longer duplicate that logic.

---

50-51: Remove unnecessary async keywords from three synchronous functions.

All three functions (getAllIcons, getIconSvgsForSet, getIconSvg) are declared async but perform only synchronous filesystem operations with no await inside. The async keyword is misleading—it suggests deferred work where none exists. The callers simply await the results, which works identically whether the functions return Promise<T> or T directly.

Remove async and adjust return types:

Diff

-export async function getAllIcons(): Promise<IconMetadata[]> {
+export function getAllIcons(): IconMetadata[] {

-export async function getIconSvgsForSet(setId: string): Promise<Record<string, string>> {
+export function getIconSvgsForSet(setId: string): Record<string, string> {

-export async function getIconSvg(setId: string, iconName: string): Promise<string | null> {
+export function getIconSvg(setId: string, iconName: string): string | null {

Also applies to: 97-98, 129-131

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/icons/reactIcons.ts` around lines 50 - 51, Remove the unnecessary
async keyword from getAllIcons, getIconSvgsForSet, and getIconSvg because they
perform only synchronous filesystem work and contain no awaits; change their
signatures to return the synchronous types (e.g., IconMetadata[] or
string[]/string as appropriate) instead of Promise<...>, delete the async
modifier from the function declarations, and update any internal return
statements to return values directly (no Promise.resolve), ensuring type
annotations for each function (getAllIcons, getIconSvgsForSet, getIconSvg)
reflect the non-Promise return type so callers and TypeScript types remain
correct.

src/pages/api/openapi.json.ts (1)

162-199: {icon-name} parameter name inconsistency and incorrect description example.

Two issues:

1. icon-name (hyphenated) is the only path parameter in this entire spec that isn't a single word or camelCase — all others use version, section, page, tab, etc. Hyphenated names can cause inconsistent variable generation in OpenAPI client generators.
2. The description repeats the same incorrect examples from index.ts: FaCircle and MdHome are Font Awesome / Material Design icons, not PatternFly icons. Update to CircleIcon or AccessibleIconIcon.

-          description:
-            'Returns actual SVG markup for the icon. Icon name: React component name (e.g., FaCircle, MdHome)',
+          description:
+            'Returns actual SVG markup for the icon. Icon name: PatternFly React component name (e.g., CircleIcon, AccessibleIconIcon)',

-            {
-              name: 'icon-name',
+            {
+              name: 'iconName',

(and update the path template /{version}/icons/{iconName} to match)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/pages/api/openapi.json.ts` around lines 162 - 199, Update the openapi
entry for getIconSvg: rename the path template from
'/{version}/icons/{icon-name}' to '/{version}/icons/{iconName}', change the path
parameter object key from 'icon-name' to 'iconName' (keeping in-path
required/schema), and update the parameter description/example to use PatternFly
icon names such as 'CircleIcon' or 'AccessibleIconIcon' instead of
'FaCircle'/'MdHome' so the operationId getIconSvg and client code generation use
a consistent camelCase parameter name.